Fix #1519: Port compiler Plugin from scalac #3438

liufengyun · 2017-11-04T00:09:58Z

Fix #1519: This PR ports compiler plugin from Scalac and adds the test infrastructure.

Scalac also supports Typer plugins, which is contentious, so it's not addressed in this PR. Current design allows easy extension to support Typer plugins if we want to do that later.

Changes from Scalac plugin:

Removed the concept PIuginComponent
Research plugin: research plugins decides where to inject their own phases

TODOs:

smarter · 2017-11-04T09:29:19Z

tests/plugins/neg/divideZero/plugin_1.scala

+    def test(tpe: String): Boolean =
+      (sym.owner eq ctx.requiredClass(tpe.toTermName)) && sym.name.show == "/"
+
+    test("scala.Int") || test("scala.Long") || test("scala.Short") || test("scala.FLoat") || test("scala.Double")


typo: FLoat -> Float

smarter · 2017-11-04T09:30:58Z

tests/plugins/neg/divideZero/plugin_1.scala

+
+  private def isNumericDivide(sym: Symbol)(implicit ctx: Context): Boolean = {
+    def test(tpe: String): Boolean =
+      (sym.owner eq ctx.requiredClass(tpe.toTermName)) && sym.name.show == "/"


You should be able to do sym.name == nme.DIV

smarter · 2017-11-04T09:33:39Z

tests/plugins/neg/divideZero/scalac-plugin.xml

@@ -0,0 +1,4 @@
+<plugin>


We should check that sbt's support for compiler plugin still works and adapt it in sbt-dotty if it doesn't: http://www.scala-sbt.org/1.x/docs/Compiler-Plugins.html We can do that by adding scripted tests in https://github.com/lampepfl/dotty/tree/master/sbt-dotty/sbt-test/sbt-dotty

smarter · 2017-11-04T09:35:22Z

tests/plugins/neg/divideZero/plugin_1.scala

+  val phaseName = name
+
+  override def init(phases: List[List[Phase]])(implicit ctx: Context): List[List[Phase]] = {
+    val (before, after) = phases.span(ps => !ps.exists(_.phaseName == "pickler"))


It would be better to not rely on strings to identify phase names and use class types instead, similar to what we do with Phase.replace in https://github.com/lampepfl/dotty/blob/master/compiler/src/dotty/tools/repl/ReplCompiler.scala#L40

Strings compose with phases coming from other compiler plugins. Class types do not.

@sjrd Good point. But can't we use Class.forName in that case?

If all compiler plugins are loaded in the same ClassLoader, then yes. But is that the case and is it desirable?

sjrd · 2017-11-04T09:49:28Z

The interface with init is both too powerful and not enough:

  override def init(phases: List[List[Phase]])(implicit ctx: Context): List[List[Phase]] = {
    val (before, after) = phases.span(ps => !ps.exists(_.phaseName == "pickler"))
    before ++ (List(this) :: after)
  }

First, it is too powerful, because it allows to reorder core phases, or phases of other plugins.

Also, it allows to insert micro-phases in existing macro-phases, which is extremely sensitive. I do not believe anyone outside of the core compiler team can write a correct micro-phase that would interact badly with the rest of its macro phase. IMO compiler plugins should only be able to insert macro phases (themselves possibly consisting of multiple micro-phases, all provided by the same plugin).

Then, it is not powerful enough, because it does not allow to correctly express dependencies between compiler plugins. With the scalac compiler plugin interface, I can write a compiler plugin B that knows about another compiler plugin A, and correctly declares its dependencies wrt. that one. With this interface, this is not possible. Let me illustrate this with a very concrete (and existing!) example:

The Scala.js JUnit compiler plugin has a phase junit-inject which needs to run between mixin and jscode (the latter being the code generation phase of the Scala.js compiler plugin. In scalac, this is easy to specify declaratively in ScalaJSJUnitPlugin. The core ScalaJSPlugin doesn't need to know about the JUnit compiler plugin. With the proposed interface for dotty, this dependency cannot be enforced without the core Scala.js plugin know about the JUnit compiler plugin. Indeed, even if the JUnit compiler plugin carefully manipulates the phases to insert itself between mixin (or whatever other phase is accurate for dotty in this case) and jscode, this only works if the init method of the JUnit plugin is called after that of the core plugin. Indeed, in the other direction, the JUnit plugin can only insert itself between mixin and delambdafy (continuing my earlier scalac-based example). Then when the core plugin's init method kicks in, since it does not know about junit-inject, it will insert itself either before junit-inject or after it. In the former case, things get broken.

This means that obtaining correct inter-plugin dependencies depends on the order in which the init methods are called. This is really bad, as this order is likely to indirectly derive from ivy resolution order.

For these reasons, I disapprove of this new interface. I strongly suggest an interface that is closer to scalac, specifying a list of afterPhases and a list of beforePhases (probably dropping rightAfterPhase because that one does not compose). And I would only allow plugins to insert macro phases. The design could provide for a possible future extension point that would allow expert compiler plugin authors to inject micro phases.

liufengyun · 2017-11-04T20:36:31Z

@sjrd Thanks a lot for sharing your experience, it's a valuable feedback 👍

We are in agreement that the new design is more powerful. As compiler plugins are for very advanced users & researchers, we may assume they use the power wisely. I hope you can also agree that the protocol/API is simpler, and the implementation is easy. In general, the usability for plugin writers is better.

For the particular problem you raised, is it possible for the junit-inject plugin to act as a facade and also add ScalaJS phases to the phase plan in the init of junit-inject?

The problem is in general about composability of plugins, which we know is very tricky: compiler plugins just don't compose easily. The Scalac approach solves some of the problems by allowing plugins to specify ordering constraints.

But the solution in Scalac is still unsatisfactory. How can a plugin knows in advance its relative order to all other plugins, given that

it's impractical for a plugin developer to test the ordering wrt. all existing plugins
new plugins continue to appear
logically, if PlugA < PlugB, should the ordering constraint be in PlugA or PlugB, or both?

The new approach instead shifts all the responsibility to programmers. This seems to be a more coherent approach to me. In practice, a few plugins will be regularly used together to solve a particular problem. Due to the frequency of the combined usage, frictions among the plugins will be detected and reduced, compatibility will improve and the relative ordering will be known. Then, it's simple to just create an orchestrate/facade plugin to compose the phase plan if the ordering is sensitive.

In the new approach, creating a facade compiler plugin is easy, just a few lines of code will do. As compiler plugins usually work with sbt, the compiler plugin facade can be authored in SBT(Build.scala) to orchestrate the phase plan, or come from some pre-defined SBT plugin. We do need to investigate to see if this approach works well with SBT and user experience is not impacted.

From the viewpoint of design, if practical composability problems can solved with the compiler taking ZERO responsibility, then it doesn't make sense for the compiler to take just PARTIAL responsibility. I assume It's always better for the boundary of responsibilities to be clear.

smarter · 2017-11-17T13:44:18Z

compiler/test/dotty/tools/dotc/plugins/PluginsTest.scala

@@ -87,7 +89,7 @@ class PluginsTest {
  def orderingTwoPlugins1 = {
    object M1 extends TestPhase {
      override val runsAfter = Set(classOf[P3d])
-      override val runsBefore = Set(M2.getClass, classOf[P7], classOf[P8])


Opened #3495 to keep track of this.

smarter · 2018-01-05T00:05:25Z

Something to keep in mind: the dynamic classloading of compiler plugins seems to have a significant performance cost, which could perhaps be avoided by better caching of classloaders, see scala/scala-dev#458

OlivierBlanvillain · 2018-01-18T17:35:30Z

@liufengyun what's is the status of this PR? If it needs a review please assign it to a reviewer :)

liufengyun · 2018-01-19T16:22:06Z

@OlivierBlanvillain This is up for review. I think @sjrd has the best experience on plugins, but it's good to get more reviews.

smarter · 2018-02-01T14:05:24Z

@nicolasstucki nicolasstucki assigned smarter and unassigned nicolasstucki 7 days ago

Unfortunately I won't have time to review this myself in the near future.

sjrd

I finally got around to reviewing this. Thanks @smarter for the ping.

sjrd · 2018-03-14T14:26:21Z

compiler/src/dotty/tools/dotc/plugins/Plugin.scala

+   *  Research plugin receives a phase plan and return a new phase plan, while
+   *  non-research plugin returns a list of phases to be inserted.
+   */
+  def research: Boolean = false


A boolean flag, and then two methods among which only one should be overridden, can be advantageously replaced by:

Making Plugin sealed, with neither of those three methods

Have 2 subtraits ResearchPlugin and StandardPlugin, each only with the one relevant abstract method.

sjrd · 2018-03-14T14:26:58Z

compiler/src/dotty/tools/dotc/plugins/Plugin.scala

+/** ...
+ *
+ *  @author Lex Spoon
+ *  @version 1.0, 2007-5-21


Is this relevant?

sjrd · 2018-03-14T14:27:58Z

compiler/src/dotty/tools/dotc/plugins/Plugin.scala

+ */
+object Plugin {
+
+  private val PluginXML = "scalac-plugin.xml"


This should be dotc-plugin.xml. Plugins for one or the other compiler are definitely not interoperable, so they should be identified by a different file.

IIRC, the "name" in that xml is unused. Maybe it's possible to ditch the xml. See Adriaan's work to avoid dependency on scala-xml at some point. It could be a properties file, or just a file containing a class name, radical simplicity. More like java's service loader. Seems a shame to drag xml config into scala 3.

Indeed, only the class name is actually used. Maybe we can assume a text file named plugins with each line corresponding to a class name.

sjrd · 2018-03-14T14:34:39Z

compiler/src/dotty/tools/dotc/plugins/Plugin.scala

+      Success[AnyClass](loader loadClass classname)
+    } catch {
+      case NonFatal(e) =>
+        Failure(new PluginLoadException(classname, s"Error: unable to load class: $classname"))


Shouldn't e.toString be included in the PluginLoadException? Currently this completely discards all the relevant information that could be used to debug an issue.

Oddly, I recall inheriting that when I touched the code. Generally e.getMessage == classname for ClassNotFoundException, but this code could be more nuanced in avoiding repeating the name, especially for other throwables or if there's a cause. I must have added the next line in this vein. You're more likely to hit NoClassDef with multiple class loaders?

sjrd · 2018-03-14T14:39:36Z

compiler/src/dotty/tools/dotc/plugins/Plugin.scala

+  }
+
+  private def loadDescriptionFromFile(f: Path): Try[PluginDescription] =
+    Try(PluginDescription.fromXML(new java.io.FileInputStream(f.jpath.toFile)))


From an API point of view, I think it would be more consistent to call this method loadDescriptionFromDirectory, and have it take the directory containing the PluginXML file (rather than the PluginXML file). This is more consistent wrt. loadDescriptionFromJar, because on a classpath, a jar ~= a directory.

It's not API, but it would be more consistent. Both methods could be local.

sjrd · 2018-03-14T15:15:08Z

compiler/src/dotty/tools/dotc/plugins/Plugin.scala

+import scala.util.{ Try, Success, Failure }
+
+trait PluginPhase extends MiniPhase {
+  def runsBefore: Set[Class[_ <: Phase]] = Set.empty


For inter-plugin dependencies, it will be impossible for a plugin A to get hold of the Class[_] value for a plugin B coming from a different jar (hence in a different ClassLoader, so not even reflectively available).

We need to use names here (also for runsAfter). Names are usually not as good as token identities, but here they offer an essential part of the expressiveness.

sjrd · 2018-03-14T15:22:32Z

sbt-dotty/sbt-test/sbt-dotty/compiler-plugin/plugin/DivideZero.scala

+    def test(tpe: String): Boolean =
+      (sym.owner eq ctx.requiredClass(tpe.toTermName)) && sym.name.show == "/"
+
+    test("scala.Int") || test("scala.Long") || test("scala.Short") || test("scala.FLoat") || test("scala.Double")


Typo: FLoat -> Float

sjrd · 2018-03-14T15:23:15Z

sbt-dotty/sbt-test/sbt-dotty/compiler-plugin/project/build.properties

@@ -0,0 +1 @@
+sbt.version=0.13.15


sjrd · 2018-03-14T15:23:48Z

tests/plugins/neg/divideZero-research/plugin_1.scala

+    def test(tpe: String): Boolean =
+      (sym.owner eq ctx.requiredClass(tpe.toTermName)) && sym.name.show == "/"
+
+    test("scala.Int") || test("scala.Long") || test("scala.Short") || test("scala.FLoat") || test("scala.Double")


Same typo on FLoat

sjrd · 2018-03-14T15:24:25Z

tests/plugins/pos/divideZero/plugin_1.scala

+    def test(tpe: String): Boolean =
+      (sym.owner eq ctx.requiredClass(tpe.toTermName)) && sym.name.show == "/"
+
+    test("scala.Int") || test("scala.Long") || test("scala.Short") || test("scala.FLoat") || test("scala.Double")


And again the same typo.

liufengyun · 2018-03-15T11:30:40Z

Thanks a lot for the review @sjrd , I'll address them this weekend.

som-snytt · 2018-03-19T00:31:43Z

compiler/src/dotty/tools/dotc/plugins/Plugin.scala

+        Plugin.load(pd.classname, loader)
+      case Failure(e)            =>
+        Failure(e)
+    }


Try.flatMap.

The original use of map is a good fit here. We are changing Success to Failure in the first two cases, Try.flatMap will not help here.

Try.flatMap allows changing Success to Failure:

(fromPaths ::: fromDirs) flatMap { case (pd, loader) if seen(pd.classname) => Failure(new PluginLoadException(pd.name, s"Ignoring duplicate plugin ${pd.name} (${pd.classname})")) case (pd, loader) if ignoring contains pd.name => Failure(new PluginLoadException(pd.name, s"Disabling plugin ${pd.name}")) case (pd, loader) => seen += pd.classname Plugin.load(pd.classname, loader) }

Thanks @Blaisorblade , sure, that's the whole purpose of flatMap/bind, a temporary short-circuit in my brain ;-)

Now to backport to scala 2.

smarter · 2018-03-20T15:23:04Z

Since this PR ports existing code, it would be good to add a note to https://github.com/lampepfl/dotty/blob/master/AUTHORS.md for proper attribution.

@sjrd

Quote @sjrd: For inter-plugin dependencies, it will be impossible for a plugin A to get hold of the `Class[_]` value for a plugin B coming from a different jar (hence in a different `ClassLoader`, so not even reflectively available). scala#3438 (comment)

sjrd

Nice :) Looks good to me :)

som-snytt · 2018-03-26T15:50:42Z

I would have smiley-faced sjrd's approval comment if github provided that function.

smarter reviewed Nov 4, 2017

View reviewed changes

liufengyun force-pushed the plugin branch from a7770d6 to 7de9503 Compare November 16, 2017 16:28

smarter reviewed Nov 17, 2017

View reviewed changes

allanrenucci assigned sjrd Nov 20, 2017

liufengyun force-pushed the plugin branch from b312fd6 to 7a86b42 Compare December 19, 2017 12:59

liufengyun force-pushed the plugin branch from 7a86b42 to 750f677 Compare January 6, 2018 15:06

OlivierBlanvillain assigned liufengyun and unassigned sjrd Jan 18, 2018

liufengyun assigned sjrd and nicolasstucki and unassigned liufengyun Jan 19, 2018

nicolasstucki assigned smarter and unassigned nicolasstucki Jan 25, 2018

smarter removed their assignment Feb 1, 2018

liufengyun force-pushed the plugin branch from 750f677 to a1f15dc Compare March 9, 2018 14:49

liufengyun assigned nicolasstucki and unassigned sjrd Mar 9, 2018

sjrd requested changes Mar 14, 2018

View reviewed changes

liufengyun unassigned nicolasstucki Mar 15, 2018

som-snytt reviewed Mar 19, 2018

View reviewed changes

liufengyun added 13 commits March 20, 2018 11:51

refactor ordering algorithm for unit test

a1a8c12

unit test plugin schedule algorithm

b05aa22

make scheduling deterministic

7fccb8e

make plugin pass bootstrapped CI

cb4d90e

add failing test

4510b18

Make scheduler deterministic by propagating odering constraints

48483cb

add transitive ordering constraint example

45ea80f

add sbt plugin test

44f2a32

remove unnecessary changes to vulpix

7fa7e72

fix rebase conflict

bcee0af

fix path after rebase

5aac55d

fix typo in tests

90d707e

split Plugin to ResearchPlugin & StandardPlugin

c49ad7f

liufengyun force-pushed the plugin branch from 4d581fe to c49ad7f Compare March 20, 2018 10:51

liufengyun added 3 commits March 20, 2018 14:49

fix more review feedback

334636e

add comment for primitive phases

7c3e289

use Try.flatMap with List.map

2737820

liufengyun added 2 commits March 20, 2018 17:05

use string names for runsAfter

2212dc0

Quote @sjrd: For inter-plugin dependencies, it will be impossible for a plugin A to get hold of the `Class[_]` value for a plugin B coming from a different jar (hence in a different `ClassLoader`, so not even reflectively available). scala#3438 (comment)

add plugin authors [CI SKIP]

d2a63fb

liufengyun force-pushed the plugin branch from f34bed0 to d2a63fb Compare March 20, 2018 16:26

liufengyun added 3 commits March 20, 2018 18:34

remove xml file

32e6faa

use plugin.properties

0e253ef

small tweak

f1a6923

sjrd approved these changes Mar 23, 2018

View reviewed changes

allanrenucci merged commit db5ca52 into scala:master Mar 26, 2018

allanrenucci deleted the plugin branch March 26, 2018 14:30

liufengyun mentioned this pull request Apr 6, 2018

Plugin detection for Dotty sbt/sbt#4073

Closed

ekrich mentioned this pull request May 10, 2018

Proposal: dotty front-end scala-native/scala-native#15

Closed

		@@ -0,0 +1 @@
		sbt.version=0.13.15

Fix #1519: Port compiler Plugin from scalac #3438

Fix #1519: Port compiler Plugin from scalac #3438

Conversation

liufengyun commented Nov 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjrd commented Nov 4, 2017

liufengyun commented Nov 4, 2017

Choose a reason for hiding this comment

smarter commented Jan 5, 2018

OlivierBlanvillain commented Jan 18, 2018

liufengyun commented Jan 19, 2018

smarter commented Feb 1, 2018

sjrd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liufengyun commented Mar 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarter commented Mar 20, 2018

sjrd left a comment

Choose a reason for hiding this comment

som-snytt commented Mar 26, 2018

liufengyun commented Nov 4, 2017 •

edited

Loading